Repository navigation
gh-49960: Allow ctypes callbacks to return struct/union types - #157974
Conversation
Documentation build overview
148 files changed ·
|
|
I'm not comfortable reviewing this right now :( Meanwhile, @ZeroIntensity is becoming the Whatever design choices are made for this feature, we probably wouldn't be able to undo them after 3.16. And the existence of monkey-patchers means that changes in |
|
Yeah, I've been slowly working through the backlog of ctypes bugs. I haven't gotten to many |
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
The approach looks good. A number of nitpicks below.
| assert(!narrow); | ||
| int ok = 0; | ||
| if (CDataObject_Check(st, result)) { | ||
| int is_inst = PyObject_IsInstance(result, restype_obj); |
There was a problem hiding this comment.
Let's use PyObject_TypeCheck instead of PyObject_IsInstance; the latter can call arbitrary Python code and probably cause some weird crashes.
| int is_inst = PyObject_IsInstance(result, restype_obj); | |
| int is_inst = PyObject_TypeCheck(result, restype_obj); |
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("error", RuntimeWarning) |
There was a problem hiding this comment.
We need to catch unraisable exceptions that result from this.
| if (!ok) { | ||
| /* Zero the buffer so the C caller sees deterministic zeros | ||
| rather than uninitialised memory. */ | ||
| memset(mem, 0, restype->size); | ||
| PyErr_Format(PyExc_TypeError, | ||
| "ctypes callback function returned unexpected " | ||
| "type %T", result); | ||
| PyErr_FormatUnraisable( | ||
| "Exception ignored while converting result " | ||
| "of ctypes callback function %R", | ||
| callable); | ||
| } |
There was a problem hiding this comment.
Up to you, but consider dropping this. We already return junk upon failure for the other paths:
>>> from ctypes import CFUNCTYPE, c_int
>>> def evil():
... return b"test"
...
>>> print(CFUNCTYPE(c_int)(evil)())
Exception ignored while converting result of ctypes callback function <function evil at 0x7fb36ad345c0>:
Traceback (most recent call last):
File "<python-input-2>", line 1, in <module>
TypeError: 'bytes' object cannot be interpreted as an integer
1813162336It would be inconsistent if struct/union returns were deterministic while other types weren't.
There was a problem hiding this comment.
Being consistent here makes sense to me. Dropped.
| PyErr_SetString(PyExc_TypeError, | ||
| "invalid result type for callback function"); | ||
| goto error; | ||
| } |
There was a problem hiding this comment.
If info->setfunc is NULL, then that means the _fields_ is inherited from a parent class, and thus can be changed later in the subclass. That would mean the size can change, which would cause a crash. Let's lock the fields here:
| } | |
| } | |
| if (info->setfunc == NULL) { | |
| // _fields_ must not be assigned after this point | |
| stginfo_set_dict_final(info); | |
| } |
| class SmallRet(Structure): | ||
| _fields_ = [("a", c_int), ("b", c_int)] |
There was a problem hiding this comment.
Consider using the new @ctypes.util.struct utility for structs in this file rather than the ugly old way.
There was a problem hiding this comment.
Please also add something to the 3.16 whatsnew.
- Check the callback result with PyObject_TypeCheck rather than PyObject_IsInstance, so a metaclass __instancecheck__ can't cause a smaller buffer to be accepted (an out-of-bounds read). - Mark the struct/union result type's _fields_ final when the callback is created, since the libffi cif depends on its layout. - Don't zero the result buffer on conversion failure, for consistency with other result types. - Detect unraisable exceptions in the repeated-call test. - Use ctypes.util.struct for test structures. - Add a What's New entry.
|
@ZeroIntensity I think I've addressed all your feedback; let me know if anything else is needed. |
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
This looks good. I'm still thinking about some of the rough edges here, but I think this is in a state where we can build off of it later. Thanks!
ctypescurrently prevents the definition of a callback function that returns a struct or union type.Unfortunately, it's extremely common for C code to define a callback that returns a structure (e.g. a Point).
Rubicon-ObjC has been carrying a monkypatch for ctypes since 2017 (originally authored by @dgelessus) because there are a small number of macOS Cocoa APIs that return a struct. This PR is a formalisation of that monkey patch, in the hope that we can eventually deprecate the patch.
Based on the historical discussion on gh-49960, struct/union returns were prohibited because of the potential memory implications - a returned struct is passed by value, but if the structure contains a pointer (e.g., a string), the pointer will be copied, but the memory being referenced won't be, which could lead to unexpected results.
The patch doesn't fix that potential memory leak issue - it essentially can't in the general case. Instead, it treats the problem as a user-side issue, handled with documentation. This was the approach suggested by two of the participants on gh-49960.
The patch here is slightly different to the one used in the Rubicon monkeypatch; in Rubicon, we have to patch both
getfuncandsetfuncbecause of the residual ctypes handling. By patching the problem in CPython itself, we're able to usesetfunc=Noneas a flag for "non-primitive return type" and use a memory copy.Fixes #49960.